Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DiagramWidget #55

Merged
merged 56 commits into from
Oct 18, 2023
Merged

DiagramWidget #55

merged 56 commits into from
Oct 18, 2023

Conversation

pbrown12303
Copy link
Contributor

Revised the DiagramWidget and its supporting widgets. Provisions for decorations on links, floating text annotations on links, resizing nodes, and dragging nodes around. Links can connect to nodes and other links (e.g. you can create a link between links) Infrastructure in place for multi-segment links, different decorations on links.

Copy link
Contributor

@Bluebugs Bluebugs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a massive amount of work. Really cool. It would be nice to have a bit more documentation.

widget/diagramwidget/.gitarchive/COMMIT_EDITMSG Outdated Show resolved Hide resolved
widget/diagramwidget/LICENSE Outdated Show resolved Hide resolved
widget/diagramwidget/README.md Show resolved Hide resolved
@charlesdaniels
Copy link
Member

So for some background context, Paul took over (with my consent) a half-completed graph widget from my staging/testing/messing around repo, fynehax. Most of the code in 931b139 is mine (he and I discussed this directly - the code was committed in this way with my explicit OK), and the subsequent commits are all Paul.

I do want to give a big shoutout to Paul for polishing this up and adding some really slick new features to make it into something really useful!

cmd/diagramdemo/main.go Outdated Show resolved Hide resolved
cmd/tabledemo/main.go Outdated Show resolved Hide resolved
cmd/viewportdemo/main.go Outdated Show resolved Hide resolved
widget/diagramwidget/connectionpad.go Show resolved Hide resolved
widget/diagramwidget/forcelayout.go Outdated Show resolved Hide resolved
widget/diagramwidget/geometry/geometry.go Show resolved Hide resolved
@charlesdaniels
Copy link
Member

Screencast.from.03-29-2023.06.39.12.PM.webm

(see attached video)

I see you made it so the nodes are resizable, which is super cool! However when resizing a node, it's possible for it to be "pushed" to the right or down, though not to the left or up. Should detect if the node is already at it's min size and do a NO-OP in this situation to avoid accidentally pushing the nodes around.

@jackclark31
Copy link

Not sure if this is the place to leave info.. I have been looking at using Fyne to work out some "GUI" ideas over using React/Web stuff.. more like Admin app stuff. One of the things I was interested in was this NodeRed style graph drag/drop nodes with connections. I saw a few posts about this particular component for Fyne.

I'll ask here.. but if there is a better place, email forum, reddit forum, etc.. please let me know.

Basically from the video this looks pretty close to what I would like to use. I am wondering if the "work" area that the widgets are placed on.. is itself placeable in any Fyne window? Is it a separate larger widget that handles zoom in/out (or if not.. planned to), scroll so more widgets can be added for bigger widget flows?

The widget itself.. is it customizable.. e.g. colors (background, text), can the main content area have pictures, text, etc positioned? Like.. the way I envision a widget is it has input ports on the left, and output ports on the right (so to speak). Can you specify the placement of the ports or connection points? As well, can you specify WHAT can be connected to (or from) so that there can be visual. indicators as you drag a connector over a connection point if it can be allowed to be connected?

Is there a potential top title area above the main box, that allows for drag handle to drag the widget around?

How customizable is the widget shape? Can it be smaller rectangular.. to allow collapsing them for example?

Lastly, can you put a box around a bunch to group them up.. and then collapse/expand that box to organize the nodes more?

Of all I listed above.. anything that is not possible.. can it be done and/or planned on?

Thank you.

@charlesdaniels
Copy link
Member

@jackclark31 I'd suggest joining the #fyne channel in the Gophers slack. If you go to fyne.io, there's a link to the channel plus a signup link for the Gophers chat all the way in the bottom on the right.

Basically from the video this looks pretty close to what I would like to use. I am wondering if the "work" area that the widgets are placed on.. is itself placeable in any Fyne window?

Should be. I didn't test for that specifically when I was writing the initial version. I'm not sure if Paul tested that specifically either. I'd be surprised if it didn't work though as it does implement the fyne.Widget interface.

Is it a separate larger widget that handles zoom in/out (or if not.. planned to)

You might be able to hack it by overriding the scaling level? Normally that's set for an entire app, not sure if it can be overridden on a per-widget basis. You should ask this in the Slack channel.

scroll so more widgets can be added for bigger widget flows?

BTW the demonstrated diagram widget already supports panning, so you can have more stuff off the edge of the screen. Fyne itself also supports both horizontally and vertically scroll-able containers.


I'm going to leave the rest to @pbrown12303 since he's taking this over. I've been away from Fyne and from this particular project long enough that I don't think I have terribly useful answers. Note that I think Paul is on vacation and unreachable at the moment.

@Bluebugs
Copy link
Contributor

I tried to ridiculously put it on the web. If you have wasm, it work: https://diagram.ddlm.me/ (just a bit slow, but still super cool).

@andydotxyz
Copy link
Member

Screencast.from.03-29-2023.06.39.12.PM.webm

(see attached video)

I see you made it so the nodes are resizable, which is super cool! However when resizing a node, it's possible for it to be "pushed" to the right or down, though not to the left or up. Should detect if the node is already at it's min size and do a NO-OP in this situation to avoid accidentally pushing the nodes around.

Good idea! You made me think and I realised the same bug exists in FyneDesk window resize!

@andydotxyz
Copy link
Member

is itself placeable in any Fyne window?

All Fyne widgets (by design) can be placed in any window on any app :).

@andydotxyz
Copy link
Member

not sure if it can be overridden on a per-widget basis.

The scale of a window is set for the whole window. Content can be manipulated to scale up using maths in the app - but only canvas primitives, not the standard widgets. You can see how I did that in the Slydes app (presentations scale to fit the space as you'd expect) https://github.com/andydotxyz/slydes

@pbrown12303
Copy link
Contributor Author

pbrown12303 commented Apr 1, 2023

@jackclark31 You are describing the exact use case I have in mind. The widget is, indeed, intended to be used as a component of a larger interface. I am using it to implement an IDE for a visual language (a separate project). What I have submitted is a work in progress - I am looking for feedback regarding the style in which it is implemented before I add more functionality.

For example I want to extend the diagram since the diagram itself is unbounded in size, I want to add zoom and pan to the DiagramWidget.

I would like to make the widgets (Diagram, Node, and Edge) extensible - I am looking for feedback on the best mechanism for doing this. I think subclassing is out (only because they already extend BaseWidget). I think I will end up adding interfaces for this purpose (which will, of course, limit the directions in which customization is possible).

You mention one extension I am working on: addint Point connection areas to nodes. The infrastructure is already there. I just need to add the interface to add them to a node. What I am wrestling with is how (what logic to use) to move them when the node scales. Do I just move them proportionally? But what happens when the interior remains fixed in size (i.e. doesn't scale)? Or is this situation not worth considering.

The diagram widget is set up with its own theme. The DiagramElements (nodes and links) take their default values from this theme. Some of these values (e.g. color) can be overridden at the individual diagram element level (as is typical in diagrams)

@andydotxyz
Copy link
Member

A widget that has extended BaseWidget can still be extended. All the core widgets are set up like this.

@jackclark31
Copy link

Can we merge this soon.. I'd like to try this out and start getting a handle on how to use it and maybe contribute if possible. Thank you.

@jackclark31
Copy link

@pbrown12303 I have a whole ton of ideas. :D. Not sure you have ever played with a video editor suite Davinci Resolve (free to download and use too.. which is amazing given how the free version is on par with high end products.. but that's out of scope :D). It has two tabs.. Fusion and Color (pages they are called). Both employ "nodes" and if you got a chance to play with that for a few minutes, you can see how you the nodes are visually very easy and pleasing to work with. They mimic Node Red (or similar). Each type of node has various ins/outs that work with some nodes and dont work with others (e.g. the visual indication of the "port" as you try to connect one node/port to it (so some sort of go code logic callback for each port would be needed to determine if the TYPE (struct, interface?) being dropped is allowed or not.. and more so.. could add functionality to allow the entire node to change color for example.. imagine you start to drag a point from a port of one node.. ALL other nodes in the work area that can NOT accept that disable.. or turn red.. so its easily visual to see that the node you're trying to connect from.. can't connect to those nodes.. and more so.. the nodes that CAN accept it might turn green, and the specific ports might flash that accept it).

As you have no doubt seen in various other tools like UML tools where they have a box.. not just a little rectangular node red style node, and its got a title area box at the top, perhaps another connected box at the bottom, and little ports along either side. Usually we're look at left to right movement, but there shouldn't be a reason any port on any side couldn't accept connections.

Grouping would be a big thing.. along with expand/collapse of groups. Imagine a tool that has multiple workflows.. that themselves connect at some point. So you can work on a few nodes.. group them (bounding box around them selects them.... right click.. popup with menu option to group, etc). So the ability to be able to expand/collapse groups, then zoom in/out and scroll to move/jump around to another group, zoom in, expand, and work again.. that is a huge time saver for various types of tools.

So.. me personally.. I love YAML format for describing things like this. You can easily define a node with various aspects like in/out ports, connections (thus dependencies), and so on. BUT.. Go not being dynamic load capable (dang it.. why is that the ONE feature they have in the std lib that is half baked.. either fix it for all platforms or remove it completely).. it would be hard without reflection or something to indicate the function in Go to call when a node is connected, or the type a given port might represent, etc.

So I am thinking we're kind of stuck with Go code only that represents nodes and functionality behind ports, etc?

But it is imperative that the overall component has some way to export the entire set of nodes (and all connections, etc) so that there is a way to save and load them. That is why I was thinking yaml (or json).

@Jacalz
Copy link
Member

Jacalz commented Apr 2, 2023

Can we merge this soon.. I'd like to try this out and start getting a handle on how to use it and maybe contribute if possible. Thank you.

We can not merge anything in an incomplete state. Before merging, it will have to be reviewed and those comments need to be addressed in the code before approval.

@pbrown12303
Copy link
Contributor Author

pbrown12303 commented Apr 3, 2023 via email

@pbrown12303
Copy link
Contributor Author

@andydotxyz R.e. the DrawingArea being public: the testing I am referring to is NOT in fyne-x: it is in the application that is using fyne-x. If I am dragging and dropping from a tree node to the drawing area and I want to test that, then the application needs to be able to simulate the related events. Similarly with selecting a toolbar button (to create something) and then tapping in the drawing area to place the created item. The alternative would be to add some "DiagramWidget.SimulateEvent() methods that would pass the events on to the (now private) drawing area. What do you think?

@pbrown12303
Copy link
Contributor Author

@andydotxyz I'm going to go with the simulated events and make DrawingArea private again. Safer.

@andydotxyz
Copy link
Member

andydotxyz commented Sep 14, 2023

@andydotxyz I'm going to go with the simulated events and make DrawingArea private again. Safer.

You don't need them either. And either way that's a lot of new APIs just for the sake of testing.
We have a test package that aims to help with testing - the widgets don't need to code it all up themselves...

For example a random object that is tappable:

obj := myType.content
test.Tap(obj.(fyne.Tappable))

No public API or simulation required. If the test package does not help with the event then we could either improve that or just call the handlers directly...

ev := fyne.DragEvent{...}
obj.Drag(ev)

for example.

And if the content is buried somewhere inside a widget we can interact with the canvas, like test.TapCanvas - or you can traverse the objects to get the item you want to test - like
inside := test.WidgetRenderer(myObj).Objects()[0].(*container.Scroll).Content

@pbrown12303
Copy link
Contributor Author

@andydotxyz Thank you! I think you have provided the answer I have been seeking. Let me try it tomorrow.

@pbrown12303
Copy link
Contributor Author

Getting close. I see two options:

  1. Leave both the drawingArea type and the drawingArea attribute of DiagramWidget private. This requires the following testing approach:
func getTappableDrawingArea(dw *diagramwidget.DiagramWidget) fyne.Tappable {
	return test.WidgetRenderer(dw).Objects()[0].(*container.Scroll).Content.(fyne.Tappable)
}

getTappableDrawingArea(diagram).Tapped(newPointEventAt(100, 100))
  1. Make DrawingArea a public class but leave the drawingArea attribute of the DiagramWidget private. This allows;
func getDrawingArea(dw *diagramwidget.DiagramWidget) *diagramwidget.DiagramArea {
	return test.WidgetRenderer(dw).Objects()[0].(*container.Scroll).Content.(fyne.Tappable)
}

getDrawingArea(diagram).Tapped(newPointEventAt(100, 100))

Which do you think is preferable?

@pbrown12303
Copy link
Contributor Author

I think I have addressed all outstanding change requests.

@andydotxyz
Copy link
Member

andydotxyz commented Oct 4, 2023

Getting close. I see two options:

  1. Leave both the drawingArea type and the drawingArea attribute of DiagramWidget private. This requires the following testing approach:
func getTappableDrawingArea(dw *diagramwidget.DiagramWidget) fyne.Tappable {
	return test.WidgetRenderer(dw).Objects()[0].(*container.Scroll).Content.(fyne.Tappable)
}

getTappableDrawingArea(diagram).Tapped(newPointEventAt(100, 100))
  1. Make DrawingArea a public class but leave the drawingArea attribute of the DiagramWidget private. This allows;
func getDrawingArea(dw *diagramwidget.DiagramWidget) *diagramwidget.DiagramArea {
	return test.WidgetRenderer(dw).Objects()[0].(*container.Scroll).Content.(fyne.Tappable)
}

getDrawingArea(diagram).Tapped(newPointEventAt(100, 100))

Which do you think is preferable?

2 is certainly not something I would go for as it requires public API to test. However maybe you are missing Option 3:

func getDrawingArea(...) fyne.CanvasObject {
    ...
}

getDrawingArea(diagram).(fyne.Tappable).Tapped(...)

Though this is all working around the setup of having tests in a different package. Putting the tests in the same package would give you access to the diagramWidget API to validate its behaviour, as per my previous message:

If you want to test items that are not public (totally reasonable) then put the test in the same package as the actual code. In the toolkit main we call these *_internal_test.go.

@pbrown12303
Copy link
Contributor Author

Your option 3 is the appropriate one in this case: the functionality being tested is not the core functionality of the Diagram Widget - it is the functionality of the callbacks that the application has added to the widgets which are going to be in the application's package, not fyne-x. Thanks for all the help here.

Is there anything else I need to do?

@andydotxyz
Copy link
Member

Your option 3 is the appropriate one in this case: the functionality being tested is not the core functionality of the Diagram Widget - it is the functionality of the callbacks that the application has added to the widgets which are going to be in the application's package, not fyne-x. Thanks for all the help here.

Is there anything else I need to do?

Happy to help.

The widget looks really great. With the info above can a few more tests be added? Happy otherwise!

@pbrown12303
Copy link
Contributor Author

I wasn't planning on adding any more tests. The ones that we were discussing were for testing my application-supplied callback functions, not functionality within the Diagram Widget itself. The test events are simply the triggers for the callback functions.

@andydotxyz
Copy link
Member

Thanks so much, with the tests all passing I'm happy. I think @Jacalz has an outstanding change request - what do you think?

@pbrown12303
Copy link
Contributor Author

Pretty sure I resolved his concern. I had upgraded the go.mod to reference fyne 2.4 and he wanted me to roll that back. I did.

@andydotxyz
Copy link
Member

Pretty sure I resolved his concern. I had upgraded the go.mod to reference fyne 2.4 and he wanted me to roll that back. I did.

I agree - but his outstanding change request is blocking the merge.

README.md Outdated
@@ -13,13 +13,13 @@ This repository holds community extensions for the [Fyne](https://fyne.io) toolk

This is in early development and more information will appear soon.

## Layouts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to try and merge this in, but realised this change to README.
A markdown document should have only 1 heading, so the changes here to re-level each item isn't right.

Jacalz
Jacalz previously approved these changes Oct 16, 2023
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the comment from Andy but otherwise approved. Well done :)

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work thanks :)

@andydotxyz andydotxyz dismissed Bluebugs’s stale review October 18, 2023 14:46

resolved, newer reviews all positive

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor changes in this latest update.
Plus could you please rename the Screenshot file?

@@ -42,7 +43,7 @@ func NewAnchoredText(text string) *AnchoredText {
at.displayedTextBinding.AddListener(at)
at.textEntry.Wrapping = fyne.TextWrapOff
// TODO After upgrade to fyne 2.4.0, uncomment the following line and add container as an imported package
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this TODO should have been removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,148 @@
<p align="center">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this block of HTML is not needed for a sub-readme

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done

@pbrown12303
Copy link
Contributor Author

Requested changes made

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, thanks so much for pulling this epic widget together!

@andydotxyz andydotxyz merged commit e7104b0 into fyne-io:master Oct 18, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants